Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiBasicTable][EuiInMemoryTable] Add new truncationText line API #7254

Merged
merged 6 commits into from
Oct 6, 2023

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Oct 4, 2023

Summary

As always, I recommend reviewing by commit to sort out feature work from cleanups.

closes #7226

Proposed API:

<EuiBasicTable
  columns={[
    {
      field: 'myField',
      name: 'My field',
      truncateText: { lines: 2 },
    }
  ]}
/>

Single (true) vs multi-line truncation:

QA

General checklist

  • Browser QA
    - [ ] Checked in both light and dark modes
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    - [ ] Updated the Figma library counterpart

@cee-chen cee-chen marked this pull request as ready for review October 4, 2023 18:39
@cee-chen cee-chen requested a review from a team as a code owner October 4, 2023 18:39
Comment on lines +47 to +52
* Indicates whether this column should truncate overflowing text content.
* - Set to `true` to enable single-line truncation.
* - To enable multi-line truncation, use a configuration object with `lines`
* set to a number of lines to truncate to.
*/
truncateText?: boolean;
truncateText?: boolean | { lines: number };
Copy link
Member Author

@cee-chen cee-chen Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/eui-team [Request for votes/feedback]

As always, naming things & consistency are the hardest part of our jobs. Here's EuiDataGrid's current line truncation API:

<EuiDataGrid
  rowHeightsOptions={{
    defaultHeight: { lineCount: 2 },
  }}
/>

And here's what the above proposed API would look like:

<EuiBasicTable
  columns={[
    {
      field: 'myField',
      name: 'My field',
      truncateText: { lines: 2 },
    }
  ]}
/>

My question for you all is whether we should change lines to lineCount to match EuiDataGrid, or let them be different (or, if someone else has a totally different naming suggestion, feel free to throw that in the ring as well).

I opted for lines when I was first writing this because it felt more natural sounding to me - I only remembered after I was done that EuiDataGrid had its own API.

FWIW, EuiDataGrid generally has a very different data structure and API architecture to our table components, so I don't think it would be a huge loss for this to be different either, but maybe I'm downplaying that. I'd love to hear people's takes!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1Copenut FYI, hoping to get this merged in for next week's release - any chance you can review this PR some time Monday morning? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm onboard with your keeping these as separate names lineCount and lines respectively. The APIs are distinct enough that I'd look them / wouldn't assume the key was the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks Trevor!

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen requested a review from 1Copenut October 6, 2023 20:25
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM. I left a comment agreeing with your reasoning for having different API key values. LMK if there's any retesting or additional review needed Monday and I'll grab it first thing.

Comment on lines +47 to +52
* Indicates whether this column should truncate overflowing text content.
* - Set to `true` to enable single-line truncation.
* - To enable multi-line truncation, use a configuration object with `lines`
* set to a number of lines to truncate to.
*/
truncateText?: boolean;
truncateText?: boolean | { lines: number };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm onboard with your keeping these as separate names lineCount and lines respectively. The APIs are distinct enough that I'd look them / wouldn't assume the key was the same.

@cee-chen cee-chen merged commit dfe59ce into elastic:main Oct 6, 2023
7 checks passed
@cee-chen cee-chen deleted the text-truncation-multi-1 branch October 6, 2023 22:33
1Copenut pushed a commit to elastic/kibana that referenced this pull request Oct 11, 2023
`v88.5.4`⏩`v89.0.0`

---

## [`89.0.0`](https://github.com/elastic/eui/tree/v89.0.0)

- Added new `pushAnimation` prop to push `EuiFlyout`s, which enables a
slide in animation ([#7239](elastic/eui#7239))
- Updated `EuiComboBox` to use `EuiInputPopover` under the hood
([#7246](elastic/eui#7246))
- Added `inputPopoverProps` to `EuiComboBox`, which allows customizing
the underlying popover
([#7246](elastic/eui#7246))
- Added a new beta `EuiTextBlockTruncate` component for multi-line
truncation ([#7250](elastic/eui#7250))
- Updated `EuiBasicTable` and `EuiInMemoryTable` to support multi-line
truncation. This can be set via `truncateText.lines` in the `columns`
prop. ([#7254](elastic/eui#7254))

**Bug fixes**

- Fixed `EuiFlexGroup` and `EuiFlexGrid`'s `m` gutter size
([#7251](elastic/eui#7251))
- Fixed focus trap rerender issues in `EuiFlyout` with memoization
([#7259](elastic/eui#7259))
- Fixed a visual bug with `EuiContextMenu`'s animation between panels
([#7268](elastic/eui#7268))

**Breaking changes**

- EUI's global body font-size now respects the `font.defaultUnits`
token. This means that the global font size will use the `rem` unit by
default, instead of `px`.
([#7182](elastic/eui#7182))
- Removed exported `accessibleClickKeys`, `comboBoxKeys`, and
`cascadingMenuKeys` services. Use the generic `keys` service instead
([#7256](elastic/eui#7256))
- Removed `EuiColorStops` due to low usage
([#7262](elastic/eui#7262))
- Removed `EuiSuggest`. We recommend using `EuiSelectable` or
`EuiComboBox` instead
([#7263](elastic/eui#7263))
- Removed `euiHeaderAffordForFixed` Sass mixin, and `$euiHeaderHeight`
and `$euiHeaderHeightCompensation` Sass variables. Use the CSS variable
`--var(euiFixedHeadersOffset, 0)` instead.
([#7264](elastic/eui#7264))

**Accessibility**

- When using `rem` or `em` font units, EUI now respects, instead of
ignoring, browser default font sizes set by end users.
([#7182](elastic/eui#7182))
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
`v88.5.4`⏩`v89.0.0`

---

## [`89.0.0`](https://github.com/elastic/eui/tree/v89.0.0)

- Added new `pushAnimation` prop to push `EuiFlyout`s, which enables a
slide in animation ([elastic#7239](elastic/eui#7239))
- Updated `EuiComboBox` to use `EuiInputPopover` under the hood
([elastic#7246](elastic/eui#7246))
- Added `inputPopoverProps` to `EuiComboBox`, which allows customizing
the underlying popover
([elastic#7246](elastic/eui#7246))
- Added a new beta `EuiTextBlockTruncate` component for multi-line
truncation ([elastic#7250](elastic/eui#7250))
- Updated `EuiBasicTable` and `EuiInMemoryTable` to support multi-line
truncation. This can be set via `truncateText.lines` in the `columns`
prop. ([elastic#7254](elastic/eui#7254))

**Bug fixes**

- Fixed `EuiFlexGroup` and `EuiFlexGrid`'s `m` gutter size
([elastic#7251](elastic/eui#7251))
- Fixed focus trap rerender issues in `EuiFlyout` with memoization
([elastic#7259](elastic/eui#7259))
- Fixed a visual bug with `EuiContextMenu`'s animation between panels
([elastic#7268](elastic/eui#7268))

**Breaking changes**

- EUI's global body font-size now respects the `font.defaultUnits`
token. This means that the global font size will use the `rem` unit by
default, instead of `px`.
([elastic#7182](elastic/eui#7182))
- Removed exported `accessibleClickKeys`, `comboBoxKeys`, and
`cascadingMenuKeys` services. Use the generic `keys` service instead
([elastic#7256](elastic/eui#7256))
- Removed `EuiColorStops` due to low usage
([elastic#7262](elastic/eui#7262))
- Removed `EuiSuggest`. We recommend using `EuiSelectable` or
`EuiComboBox` instead
([elastic#7263](elastic/eui#7263))
- Removed `euiHeaderAffordForFixed` Sass mixin, and `$euiHeaderHeight`
and `$euiHeaderHeightCompensation` Sass variables. Use the CSS variable
`--var(euiFixedHeadersOffset, 0)` instead.
([elastic#7264](elastic/eui#7264))

**Accessibility**

- When using `rem` or `em` font units, EUI now respects, instead of
ignoring, browser default font sizes set by end users.
([elastic#7182](elastic/eui#7182))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiBasicTable/Flex layouts] Support for multi line truncation.
4 participants